-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove unnecessary optionality #271
Conversation
Benchmarks show this change is performance neutral. Three runs from before:
After:
|
@@ -308,7 +308,7 @@ class VideoDecoder { | |||
// The desired position of the cursor in the stream. We send frames >= | |||
// this pts to the user when they request a frame. | |||
// We set this field if the user requested a seek. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment stale now?
Alternative change would be to not initialize this to 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good catch. We can make it accurate with a one-word change.
Not initializing this variable to be 0 would actually have the same behavior as my change, although my change is simpler code. That is, we would use INT64_MIN
until a seek happens in both cases.
4f35de4
to
5baa9fc
Compare
Because we set this value to 0 initially, it is always there. Since it's always there, there's no reason for it to be an optional. However, 0 is not a great default, since streams can technically have pts values less than 0. So let's just set it to be
INT64_MIN
, which is what we thought was appropriate if the value was not set (even though it always was!).